fix(channel): Prevent leaked SocketMap references during reinitializa… by darion-yaphet · Pull Request #3433 · apache/brpc

darion-yaphet · 2026-08-10T15:20:32Z

What problem does this PR solve?

Problem Summary:

A direct Channel could be initialized more than once. Each successful initialization inserted a SocketMap entry, while destruction released only the key derived from
the final configuration, leaving earlier entries unbalanced.

What is changed and the side effects?

Changed:

Reject every Init() call after the first successful initialization.
Continue allowing Init() retries after a failed attempt.
Add regression tests for successful-init rejection and failed-init retry.
Document the single-shot Channel initialization contract in the public header and English/Chinese client guides.

Side effects:

A Channel's target and options are immutable after Init() succeeds; later Init() calls return -1.
Callers that need another target or configuration must create a new Channel.
Channel::Init() remains non-thread-safe.
No per-Channel socket-map lifecycle state or heap allocation is needed.
Compatibility: callers that previously reinitialized a Channel must create a replacement Channel instead; repeated initialization was not resource-safe.

Copilot

Pull request overview

This PR fixes brpc::Channel reinitialization for direct (single-server) channels by ensuring the exact SocketMapKey used during initialization is preserved and properly released, preventing leaked SocketMap references and avoiding partial state overwrites when reinitialization fails.

Changes:

Track and remove the precise SocketMapKey for the active single-server channel state to prevent stale SocketMap references after reinit/destruction.
Rework initialization to build ChannelOptions/protocol function pointers locally and only commit to the Channel on success, keeping prior state intact on failure.
Add unit tests covering repeated direct initialization and failed reinitialization behavior.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
test/brpc_channel_unittest.cpp	Adds regression tests for socket-map reference release on reinit and state preservation on failed reinit.
src/brpc/channel.h	Introduces internal single-server state storage (`unique_ptr`) and a reset helper for single-server lifecycle.
src/brpc/channel.cpp	Implements stored `SocketMapKey` lifecycle management and commits init state only after success to avoid partial overwrites.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/brpc/channel.cpp:497

Init(ns_url, lb_name, ...) commits _options and protocol function pointers before validating client_host and before lb->Init(). If client_host is invalid or lb->Init() fails, Init() returns -1 but the channel’s options/protocol callbacks have already been overwritten while _scheme/_service_name/_lb remain unchanged. This creates a partially-updated channel configuration after a failed reinit.

Suggested fix: keep the parsed URL pieces and initialized_options local, and only assign _options/callbacks and swap _scheme/_service_name after lb->Init() succeeds (similar to how you defer ResetSingleServer() today).

    _options = initialized_options.options;
    _serialize_request = initialized_options.serialize_request;
    _pack_request = initialized_options.pack_request;
    _get_method_name = initialized_options.get_method_name;
    _preferred_index = initialized_options.preferred_index;

src/brpc/channel.cpp:416

InitSingle() assigns _options and protocol function pointers before validating server_addr_and_port.port / client_host and before the SocketMap insert+commit block. If any of the subsequent checks fail and the function returns -1, the channel keeps using the previously-initialized socket-map entry but its options are partially overwritten, leaving the channel in an inconsistent state after a failed reinitialization (contrary to the PR description’s “keep prior state intact on failure”).

Consider keeping initialized_options purely local and only committing _options/_serialize_request/etc after all validation succeeds and you’re ready to ResetSingleServer() + swap in the new state (or add a scoped rollback guard to restore the previous members on every early-return).

This issue also appears on line 493 of the same file.

    _options = initialized_options.options;
    _serialize_request = initialized_options.serialize_request;
    _pack_request = initialized_options.pack_request;
    _get_method_name = initialized_options.get_method_name;
    _preferred_index = initialized_options.preferred_index;

test/brpc_channel_unittest.cpp:2353

This test currently asserts that channel.options().client_host equals the invalid client_host from a failed Init() call. If the intent is to keep the prior channel state intact when reinitialization fails, options should remain unchanged after the failed init.

Capturing the original client_host and asserting it remains the same also avoids locking in inconsistent runtime behavior where options() no longer matches the socket that is actually in use.

        EXPECT_EQ(endpoint, channel._server_address);
        EXPECT_EQ(original_id, channel._server_id);
        EXPECT_EQ(invalid_options.client_host, channel.options().client_host);
        EXPECT_TRUE(channel.SingleServer());

wwbmmm · 2026-08-15T05:50:43Z

LGTM

chenBright

Instead of managing the SocketMap key lifecycle for re-init, I'd suggest simply rejecting re-initialization once Init() has succeeded. A Channel that's initialized exactly once inserts exactly one socket entry and never mutates _options afterward, so the signature recomputed in ~Channel() always matches the insertion key and the reference stays balanced — no extra state needed. Failed inits before the first success can still be retried.

darion-yaphet · 2026-08-17T17:15:24Z

Instead of managing the SocketMap key lifecycle for re-init, I'd suggest simply rejecting re-initialization once Init() has succeeded. A Channel that's initialized exactly once inserts exactly one socket entry and never mutates _options afterward, so the signature recomputed in ~Channel() always matches the insertion key and the reference stays balanced — no extra state needed. Failed inits before the first success can still be retried.

Thanks for the suggestion! That makes total sense.

Rejecting re-initialization after a successful Init() keeps Channel simple without extra heap allocation or state tracking, while still allowing failed Init() attempts to be retried. I will update the PR to follow this approach.

darion-yaphet · 2026-08-25T05:01:09Z

Still following this PR? @chenBright

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

Suppressed comments (1)

test/brpc_channel_unittest.cpp:2357

Same issue as above: this only checks that SocketMapFind(SocketMapKey(endpoint)) fails after destruction, but does not first confirm that this key was actually present during the successful Init. Adding an in-scope ASSERT_EQ(0, SocketMapFind(SocketMapKey(endpoint), &id)) (and optionally id == channel._server_id) would ensure the test is validating removal of the correct key rather than passing due to a key/signature mismatch.

    brpc::SocketId id;
    EXPECT_NE(0, brpc::SocketMapFind(brpc::SocketMapKey(endpoint), &id));
}

darion-yaphet · 2026-08-25T07:49:27Z

+        ASSERT_EQ(0, channel.Init(first_endpoint, NULL));
+        ASSERT_EQ(-1, channel.Init(first_endpoint, NULL));
+        ASSERT_EQ(-1, channel.Init(second_endpoint, NULL));
+        ASSERT_EQ(-1, channel.Init("unknown://unknown", "rr", NULL));
+    }


The code and test had already switched to rejecting reinitialization after a successful Init(), but the GitHub PR description was not updated accordingly, which caused the inconsistency.

+    {
+        brpc::Channel channel;
+        ASSERT_EQ(0, channel.Init(first_endpoint, NULL));
+        ASSERT_EQ(-1, channel.Init(first_endpoint, NULL));
+        ASSERT_EQ(-1, channel.Init(second_endpoint, NULL));
+        ASSERT_EQ(-1, channel.Init("unknown://unknown", "rr", NULL));
+    }
+
+    brpc::SocketId id;
+    EXPECT_NE(0, brpc::SocketMapFind(brpc::SocketMapKey(first_endpoint), &id));
+    EXPECT_NE(0, brpc::SocketMapFind(brpc::SocketMapKey(second_endpoint), &id));


darion-yaphet · 2026-08-25T08:02:16Z

+    if (_server_id != INVALID_SOCKET_ID || _lb != NULL) {
+        LOG(ERROR) << "Channel=" << this << " has already been initialized";
+        return -1;
+    }


It has been resolved previously.

yanglimingcn · 2026-08-25T09:49:36Z

Turn multiple commits into one

…lanced Reject re-initialization once Channel::Init() has succeeded. This ensures a Channel instance only inserts into SocketMap at most once and preserves its options and signature intact, guaranteeing that ~Channel() always balances the insertion without requiring extra state tracking. Failed inits before the first successful initialization can still be retried.

yanglimingcn · 2026-08-25T11:45:34Z

LGTM

chenBright

LGTM

chenBright · 2026-08-25T11:52:44Z

Turn multiple commits into one

@wwbmmm You can choose "Squash and merge" to merge commits during the merge process, which makes the commit history clearer.

wwbmmm · 2026-08-25T12:43:30Z

Turn multiple commits into one

@wwbmmm You can choose "Squash and merge" to merge commits during the merge process, which makes the commit history clearer.

OK

chenBright requested a lite review from Copilot August 10, 2026 15:33

Copilot started reviewing on behalf of chenBright August 10, 2026 15:33 View session

Copilot AI reviewed Aug 10, 2026

View reviewed changes

darion-yaphet force-pushed the fix/channel-reinit-socketmap-lifecycle branch from 416cdb9 to 9f169d0 Compare August 10, 2026 17:32

chenBright requested a lite review from Copilot August 11, 2026 03:16

Copilot started reviewing on behalf of chenBright August 11, 2026 03:17 View session

Copilot AI reviewed Aug 11, 2026

View reviewed changes

chenBright reviewed Aug 16, 2026

View reviewed changes

darion-yaphet force-pushed the fix/channel-reinit-socketmap-lifecycle branch from 9f169d0 to c0aaf32 Compare August 17, 2026 17:38

chenBright requested a lite review from Copilot August 25, 2026 06:15

Copilot started reviewing on behalf of chenBright August 25, 2026 06:17 View session

Copilot AI reviewed Aug 25, 2026

View reviewed changes

darion-yaphet force-pushed the fix/channel-reinit-socketmap-lifecycle branch from e75913f to 1a4dfa4 Compare August 25, 2026 07:58

darion-yaphet force-pushed the fix/channel-reinit-socketmap-lifecycle branch from 1a4dfa4 to 246a1c7 Compare August 25, 2026 10:49

chenBright approved these changes Aug 25, 2026

View reviewed changes

chenBright merged commit f620393 into apache:master Aug 25, 2026
15 checks passed

Uh oh!

Conversation

darion-yaphet commented Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and the side effects?

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

wwbmmm commented Aug 15, 2026

Uh oh!

chenBright left a comment

Choose a reason for hiding this comment

Uh oh!

darion-yaphet commented Aug 17, 2026

Uh oh!

darion-yaphet commented Aug 25, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

darion-yaphet Aug 25, 2026

Choose a reason for hiding this comment

Uh oh!

darion-yaphet Aug 25, 2026

Choose a reason for hiding this comment

Uh oh!

yanglimingcn commented Aug 25, 2026

Uh oh!

yanglimingcn commented Aug 25, 2026

Uh oh!

chenBright left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chenBright commented Aug 25, 2026

Uh oh!

wwbmmm commented Aug 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

darion-yaphet commented Aug 10, 2026 •

edited

Loading